refactor: use chat.update endpoint instead of updateMessage method #37138

cardoso · 2025-10-03T19:47:21Z

The current /v1/chat.update REST API endpoint only supports updating messages via the text parameter, but it doesn't support the content parameter needed for End-to-End Encrypted (E2EE) messages or the e2eMentions parameter for encrypted mentions.

This limitation affects mobile clients that need to:

Edit encrypted messages using the upcoming new encryption format in feat: e2ee security hardening #36942 .
Handle mentions in encrypted rooms (e2eMentions)

Currently, mobile clients cannot properly edit encrypted messages because:

The API only accepts text parameter, not content.
There's no support for e2eMentions parameter.
Moving back to Meteor methods (/v1/method.call/updateMessage) in mobile is not an option as the team is moving away from Meteor methods.

Proposed changes (including videos or screenshots)

Current Behavior

Web: Uses /v1/method.call/updateMessage Meteor method (supports both msg and content).
Mobile: Uses /v1/chat.update REST API (only supports text, not content).
API Schema: Only accepts text, roomId, msgId, previewUrls, and customFields.

Desired Behavior

Update the /v1/chat.update endpoint to support both traditional and encrypted message formats.

Issue(s)

https://rocketchat.atlassian.net/browse/ESH-39

Steps to test or reproduce

Try editing a message in an encrypted channel on mobile to include a channel or user mention. It will not process or display the mentions.

Further comments

Summary by CodeRabbit

New Features
- Edit messages via REST with plain-text or encrypted payloads; encrypted edits support user and channel mentions.
Refactor
- Message editing consolidated behind a single chat.update endpoint with distinct text vs encrypted request shapes; client switched to the REST endpoint and payloads follow the new schemas.
Bug Fixes
- Server enforces that only encrypted messages may use content updates (returns 400 otherwise).
Tests
- Added e2e tests for encrypted-message edits (including mentions) and API validation; updated fixtures handling.

dionisio-bot · 2025-10-03T19:47:24Z

Looks like this PR is not ready to merge, because of the following issues:

This PR is targeting the wrong base branch. It should target 7.12.0, but it targets 7.11.0

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

changeset-bot · 2025-10-03T19:47:26Z

⚠️ No Changeset found

Latest commit: 2d027d4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

coderabbitai · 2025-10-03T19:48:49Z

Walkthrough

Adds schema-validated /v1/chat.update supporting text or encrypted content (with e2eMentions), migrates web client from Meteor method to REST, expands server updateMessage types to accept content-shaped payloads, updates REST typings/schema, adds E2E tests for encrypted edits, and centralizes fixture path helper.

Changes

Cohort / File(s)	Summary
REST API: chat.update endpoint `apps/meteor/app/api/server/v1/chat.ts`	Adds a schema-driven `chat.update` POST route (under `chat.pinMessage`), authenticates, validates a oneOf (text or encrypted) payload via `isChatUpdateProps`, builds update payload (text/customFields/previewUrls or content/e2eMentions), delegates to `executeUpdateMessage` through air-gapped validation, re-fetches/normalizes the updated message, and returns `{ message, success }`. Removes the duplicate earlier `chat.update` implementation.
Server update logic & method signatures `apps/meteor/app/lib/server/functions/updateMessage.ts`, `apps/meteor/app/lib/server/methods/updateMessage.ts`	Expands `message` parameter types to accept either `AtLeast<IMessage, '_id'
Client migration to REST `apps/meteor/client/lib/chats/data.ts`	Replaces `sdk.call('updateMessage', ...)` Meteor method usage with `sdk.rest.post('/v1/chat.update', ...)`. Payload shape chosen conditionally via `isEncryptedMessageContent`: encrypted messages send `content` and optional `e2eMentions`; non-encrypted send `text`, `customFields?`, and `previewUrls`.
REST typings & schema `packages/rest-typings/src/v1/chat.ts`	Adds `IChatUpdate`, `IChatUpdateText`, `IChatUpdateEncrypted`; changes `ChatUpdate` to a union of text/encrypted variants; updates `ChatUpdateSchema` to `oneOf` with strict validation, makes `customFields` optional for text variant, introduces `content` and `e2eMentions` structure for encrypted variant, and enforces `additionalProperties: false`.
E2E tests: encrypted edits & endpoint checks `apps/meteor/tests/e2e/e2e-encryption/e2ee-encrypted-channels.spec.ts`, `apps/meteor/tests/e2e/messaging.spec.ts`, `apps/meteor/tests/end-to-end/api/chat.ts`	Adds E2E tests for editing encrypted messages (including mentions), updates network wait predicates to observe `/api/v1/chat.update` POST 200 instead of the Meteor method, and adds an API test asserting 400 when trying to update non-encrypted messages using `content`.
E2E test utilities / fixtures `apps/meteor/tests/e2e/page-objects/fragments/home-content.ts`	Adds `FIXTURES_PATH` and exported `getFilePath(fileName: string): string` using Node `path`; replaces inline fixture paths with the helper for file reads/uploads.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Client as Web Client
  participant API as REST /v1/chat.update
  participant Auth as AuthN/AuthZ
  participant Schema as isChatUpdateProps
  participant Service as executeUpdateMessage
  participant DB as Messages Store

  Client->>API: POST /v1/chat.update { roomId, msgId, text | content, ... }
  API->>Auth: validate session & permissions
  Auth-->>API: OK
  API->>Schema: validate oneOf (text OR encrypted)
  Schema-->>API: Valid
  alt Text update
    API->>Service: uid, { msg, customFields?, previewUrls? }
  else Encrypted update
    API->>Service: uid, { content, e2eMentions? }
  end
  Service->>DB: apply update
  DB-->>Service: updated message
  Service-->>API: normalized message
  API-->>Client: { success: true, message }

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

feat: adds federation files support #36842 — Related federation/encrypted-content types and guards used by the new encrypted payload handling and client branching.

Suggested labels

stat: ready to merge, stat: QA assured

Poem

I twitch my whiskers at endpoints new,
Hopped from Meteor to REST—off I flew!
Cipher or text, I nibble and mend,
Mentions stay snug when edits end.
Carrot cheers—tests pass with a hop! 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues Check	⚠️ Warning	The PR implements the union‐based REST API schema in rest‐typings, updates the chat.update handler and both updateMessage functions, and migrates the web client to use chat.update as specified in ESH-39, with end‐to‐end and API tests verifying encrypted message editing and error handling; however, no unit tests for the new schema validation were added and it is unclear whether the v2 encryption parameters (iv and kid) are fully supported.	Add focused unit tests for ChatUpdateSchema covering both text and encrypted variants and verify that the schema accepts and validates the v2 encryption fields (iv and kid) as required by ESH-39.
Out of Scope Changes Check	⚠️ Warning	The PR includes a refactoring of E2E page-object fixtures—adding getFilePath in home-content.ts to centralize file paths—that is unrelated to the chat.update API changes defined in ESH-39 and introduces out-of-scope functionality for this ticket.	Extract the fixture path helper refactoring into a separate pull request so that this PR remains focused solely on the chat.update endpoint implementation.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title succinctly describes the primary change—migrating from the Meteor updateMessage method to the chat.update REST endpoint—and is concise and clear without extraneous detail.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch refactor/chat.update-ESH-39

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

codecov · 2025-10-03T20:02:21Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.38%. Comparing base (16da6b3) to head (2d027d4).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop   #37138   +/-   ##
========================================
  Coverage    67.37%   67.38%           
========================================
  Files         3288     3288           
  Lines       111814   111815    +1     
  Branches     20419    20439   +20     
========================================
+ Hits         75335    75342    +7     
+ Misses       33789    33785    -4     
+ Partials      2690     2688    -2

Flag	Coverage Δ
e2e	`57.29% <66.66%> (-0.02%)`	⬇️
unit	`71.39% <ø> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

coderabbitai

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

apps/meteor/app/lib/server/methods/updateMessage.ts (1)
16-40: Fix early-return logic to handle content-based updates correctly.

The early-return logic at lines 37-39 assumes message.msg exists:
const msgText = originalMessage?.attachments?.[0]?.description ?? originalMessage.msg;
if (msgText === message.msg && !previewUrls && !message.customFields) {
  return;
}
For content-based updates (where message.msg is undefined), this comparison will incorrectly trigger the early return when msgText is defined but message.msg is undefined, preventing legitimate content updates.

Apply this diff to fix the logic:
 	const msgText = originalMessage?.attachments?.[0]?.description ?? originalMessage.msg;
-	if (msgText === message.msg && !previewUrls && !message.customFields) {
+	if ('content' in message || (msgText === message.msg && !previewUrls && !message.customFields)) {
-		return;
+		if ('content' in message) {
+			// Content-based updates should always proceed
+		} else {
+			return;
+		}
 	}
Or more simply:
 	const msgText = originalMessage?.attachments?.[0]?.description ?? originalMessage.msg;
-	if (msgText === message.msg && !previewUrls && !message.customFields) {
+	if (!('content' in message) && msgText === message.msg && !previewUrls && !message.customFields) {
 		return;
 	}

🧹 Nitpick comments (1)

apps/meteor/tests/e2e/page-objects/fragments/home-content.ts (1)
2-2: Consider using absolute paths for robustness.

The current implementation uses relative(process.cwd(), ...) to create paths relative to the working directory. While this works if tests always run from the repository root, it's fragile and could break if the working directory changes.

For more reliable path resolution, use absolute paths directly:
-import { resolve, join, relative } from 'node:path';
+import { resolve, join } from 'node:path';

-const FIXTURES_PATH = relative(process.cwd(), resolve(__dirname, '../../fixtures/files'));
+const FIXTURES_PATH = resolve(__dirname, '../../fixtures/files');
This eliminates dependency on the working directory and works regardless of where tests are executed from. Both fs.readFile() and Playwright's setInputFiles() handle absolute paths reliably.

Also applies to: 8-12

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between f627e67 and 50e314c.

📒 Files selected for processing (11)

apps/meteor/app/api/server/v1/chat.ts (4 hunks)
apps/meteor/app/lib/server/functions/updateMessage.ts (1 hunks)
apps/meteor/app/lib/server/methods/updateMessage.ts (1 hunks)
apps/meteor/client/lib/chats/data.ts (1 hunks)
apps/meteor/client/startup/useRedirectToSetupWizard.ts (1 hunks)
apps/meteor/tests/e2e/e2e-encryption/e2ee-encrypted-channels.spec.ts (1 hunks)
apps/meteor/tests/e2e/messaging.spec.ts (2 hunks)
apps/meteor/tests/e2e/page-objects/fragments/home-content.ts (5 hunks)
packages/ddp-client/src/legacy/RocketchatSDKLegacy.ts (2 hunks)
packages/ddp-client/src/legacy/types/SDKLegacy.ts (2 hunks)
packages/rest-typings/src/v1/chat.ts (0 hunks)

💤 Files with no reviewable changes (1)

packages/rest-typings/src/v1/chat.ts

🧰 Additional context used

📓 Path-based instructions (4)

apps/meteor/tests/e2e/**/*.spec.ts

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

apps/meteor/tests/e2e/**/*.spec.ts: All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Use descriptive test names that clearly communicate expected behavior
Use test.beforeAll() and test.afterAll() for setup and teardown
Use test.step() to organize complex test scenarios
Group related tests in the same file
Utilize Playwright fixtures (test, page, expect) consistently
Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Maintain test isolation between test cases
Ensure a clean state for each test execution
Ensure tests run reliably in parallel without shared state conflicts

Files:

apps/meteor/tests/e2e/messaging.spec.ts
apps/meteor/tests/e2e/e2e-encryption/e2ee-encrypted-channels.spec.ts

apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}: Write concise, technical TypeScript/JavaScript with accurate typing
Follow DRY by extracting reusable logic into helper functions or page objects
Avoid code comments in the implementation

Files:

apps/meteor/tests/e2e/messaging.spec.ts
apps/meteor/tests/e2e/page-objects/fragments/home-content.ts
apps/meteor/tests/e2e/e2e-encryption/e2ee-encrypted-channels.spec.ts

apps/meteor/tests/e2e/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

apps/meteor/tests/e2e/**/*.{ts,tsx}: Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Store commonly used locators in variables/constants for reuse
Use page.waitFor() with specific conditions and avoid hardcoded timeouts
Implement proper wait strategies for dynamic content
Follow the Page Object Model pattern consistently

Files:

apps/meteor/tests/e2e/messaging.spec.ts
apps/meteor/tests/e2e/page-objects/fragments/home-content.ts
apps/meteor/tests/e2e/e2e-encryption/e2ee-encrypted-channels.spec.ts

apps/meteor/tests/e2e/page-objects/**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

Utilize and place Page Object implementations under apps/meteor/tests/e2e/page-objects/

Files:

apps/meteor/tests/e2e/page-objects/fragments/home-content.ts

🧠 Learnings (6)